Skip to content
This repository has been archived by the owner on Apr 14, 2021. It is now read-only.

Add config variable and check for platform warnings #6309

Merged
merged 4 commits into from
Mar 1, 2018

Conversation

agrim123
Copy link
Contributor

@agrim123 agrim123 commented Feb 25, 2018

Thanks so much for the contribution!
To make reviewing this PR a bit easier, please fill out answers to the following questions.

What was the end-user problem that led to this PR?

The user needed a way to turn off platform warnings.

What was your diagnosis of the problem?

Creating a config variable to solve the above problem.

What is your fix for the problem, implemented in this PR?

Added a key disable_platform_warnings in settings and placed check at the relevant place to disable warnings.

Why did you choose this fix out of the possible options?

We will by default show warnings but the user might want to disable them, so using a config variable looked a good option.

Fixes #6124

@ghost
Copy link

ghost commented Feb 25, 2018

Thanks for opening a pull request and helping make Bundler better! Someone from the Bundler team will take a look at your pull request shortly and leave any feedback. Please make sure that your pull request has tests for any changes or added functionality.

We use Travis CI to test and make sure your change works functionally and uses acceptable conventions, you can review the current progress of Travis CI in the PR status window below.

If you have any questions or concerns that you wish to ask, feel free to leave a comment in this PR or join our #bundler channel on Slack.

For more information about contributing to the Bundler project feel free to review our CONTRIBUTING guide

@colby-swandale
Copy link
Member

Thanks for helping make Bundler better! A couple of things that need to be done before we can merge this. This is going to need a test to make sure this is behaving as expected and so that we don't regress in the future, you will want to look at this test file. You will also need to document the new configuration option to the bundle config man page

it "prints a helpful warning when a dependency is unused on any platform" do
simulate_platform "ruby"
simulate_ruby_engine "ruby"
context "when disable_platform_warnings is false (by default)" do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test doesn't need a context blocked wrapping around it as the setting is set in Bundler automatically.

context "when disable_platform_warnings is true" do
before { bundle! "config disable_platform_warnings true" }

it "doesnot print the warning when a dependency is unused on any platform" do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does not

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad!

bundle! "install"

expect(out).not_to include <<-O.strip
The dependency #{Gem::Dependency.new("rack", ">= 0")} will be unused by any of the platforms Bundler is installing for. Bundler is installing for ruby but the dependency is only for x86-mingw32, x86-mswin32, x64-mingw32, java. To add those platforms to the bundle, run `bundle lock --add-platform x86-mingw32 x86-mswin32 x64-mingw32 java`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a lot to match exactly against in the output which could easily be missed if we change the test or behavior. I would match with:

expect(out).to_not include(/The dependency (.*) will be unused/)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool. Will change it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, using include is throwing a TypeError

TypeError:
       no implicit conversion of Regexp into String

How about using match?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, that was a typo on my part.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using

expect(out).not_to match(/The dependency (.*) will be unused/)

does the trick. Is it good?

Copy link
Member

@colby-swandale colby-swandale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is nearly ready to merge, just one more thing to fix up.

@@ -166,6 +166,8 @@ learn more about their operation in [bundle install(1)][bundle-install(1)].
When set, Gemfiles containing multiple sources will produce errors
instead of warnings.
Use `bundle config --delete disable_multisource` to unset.
* `disable_platform_warnings` (`BUNDLE_DISABLE_PLATFORM_WARNINGS`):
Whether Bundler should show platform warnings.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would reword this to be a bit more explicit, something like:

Disable warnings during bundle install when a dependency is unused on the current platform

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done 😄

@colby-swandale colby-swandale added this to the 1.16.2 milestone Mar 1, 2018
@colby-swandale
Copy link
Member

Thanks! @bundlerbot r+

@bundlerbot
Copy link
Collaborator

📌 Commit 5e73a89 has been approved by colby-swandale

@bundlerbot
Copy link
Collaborator

⌛ Testing commit 5e73a89 with merge 23dfadc...

bundlerbot added a commit that referenced this pull request Mar 1, 2018
…ndale

Add config variable and check for platform warnings

Thanks so much for the contribution!
To make reviewing this PR a bit easier, please fill out answers to the following questions.

### What was the end-user problem that led to this PR?

The user needed a way to turn off platform warnings.

### What was your diagnosis of the problem?

Creating a config variable to solve the above problem.

### What is your fix for the problem, implemented in this PR?

Added a key `disable_platform_warnings` in settings and placed check at the relevant place to disable warnings.

### Why did you choose this fix out of the possible options?

We will by default show warnings but the user might want to disable them, so using a config variable looked a good option.

Fixes #6124
@bundlerbot
Copy link
Collaborator

☀️ Test successful - status-travis
Approved by: colby-swandale
Pushing 23dfadc to master...

@bundlerbot bundlerbot merged commit 5e73a89 into rubygems:master Mar 1, 2018
@colby-swandale colby-swandale modified the milestones: 1.16.2, 1.17.0 Apr 4, 2018
colby-swandale pushed a commit that referenced this pull request Sep 20, 2018
…ndale

Add config variable and check for platform warnings

Thanks so much for the contribution!
To make reviewing this PR a bit easier, please fill out answers to the following questions.

### What was the end-user problem that led to this PR?

The user needed a way to turn off platform warnings.

### What was your diagnosis of the problem?

Creating a config variable to solve the above problem.

### What is your fix for the problem, implemented in this PR?

Added a key `disable_platform_warnings` in settings and placed check at the relevant place to disable warnings.

### Why did you choose this fix out of the possible options?

We will by default show warnings but the user might want to disable them, so using a config variable looked a good option.

Fixes #6124

(cherry picked from commit 23dfadc)
colby-swandale pushed a commit that referenced this pull request Oct 5, 2018
…ndale

Add config variable and check for platform warnings

Thanks so much for the contribution!
To make reviewing this PR a bit easier, please fill out answers to the following questions.

### What was the end-user problem that led to this PR?

The user needed a way to turn off platform warnings.

### What was your diagnosis of the problem?

Creating a config variable to solve the above problem.

### What is your fix for the problem, implemented in this PR?

Added a key `disable_platform_warnings` in settings and placed check at the relevant place to disable warnings.

### Why did you choose this fix out of the possible options?

We will by default show warnings but the user might want to disable them, so using a config variable looked a good option.

Fixes #6124

(cherry picked from commit 23dfadc)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The ability to silence platform warnings
3 participants